Fix ComObject finalizer crash from OLE DB error RCWs#130906
Conversation
OleDbError's constructor created per-record IErrorInfo and ISQLErrorInfo RCWs but released them only on the success path. A provider returning a failure HRESULT from a non-[PreserveSig] call (GetErrorInfo, GetSQLInfo) threw mid-extraction, letting the RCW escape to ~ComObject() and be released on the GC finalizer thread. For a thread-affinitized OLE DB error object that off-thread Marshal.Release faults (0xC0000005). Wrap both RCWs in try/finally so they are always FinalRelease'd on the calling thread regardless of exceptions. Fixes dotnet#125221 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
AaronRobinsonMSFT
added
area-System.Data.OleDB
and removed
area-System.Runtime.InteropServices
labels
|
@dotnet/interop-contrib PTAL |
|
Tagging subscribers to this area: @SamMonoRT, @dotnet/efteam |
There was a problem hiding this comment.
Pull request overview
Fixes a reliability issue in source-generated COM interop usage for OLE DB error handling by ensuring thread-affine COM wrappers are deterministically released on the originating thread (instead of being left for finalization), and adds a DEBUG-only diagnostic to help catch non-agile COM objects being used with the free-threaded COM strategy.
Changes:
- Ensure per-record
IErrorInfoandISQLErrorInfoCOM wrappers inOleDbErrorare always released viatry/finally. - Add a DEBUG-only Windows agility probe/assertion to
FreeThreadedStrategyto detect non-free-threaded COM objects early.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime.InteropServices/src/System/Runtime/InteropServices/Marshalling/FreeThreadedStrategy.cs | Adds DEBUG-only agility probing/assertion for COM objects used with the free-threaded IUnknown strategy. |
| src/libraries/System.Data.OleDb/src/OleDbError.cs | Wraps COM error object usage in try/finally to ensure RCWs are released on the calling thread even when extraction throws. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 2
jtschuster
left a comment
jtschuster
left a comment
There was a problem hiding this comment.
Not familiar enough with COM to know if IsFreeThreaded is correct in all cases, but it should only be firing in Debug builds / CI, and the try/catch changes LGTM.
…s to ensure safety when called from the GC finalizer thread
… use MemberFunction attribute
|
It looks like the test failure got through because runtime pipeline just completely failed to start: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1513486&view=results |
Reverts the DEBUG-only free-threaded assert added to `FreeThreadedStrategy` in #130906. The assert was overly strict: `FreeThreadedStrategy` is also valid for apartment-affinitized COM objects when the current thread already has the correct apartment type, so the agility probe produced false assert failures. Also clarifies documentation on `StrategyBasedComWrappers.DefaultIUnknownStrategy` and adds a class comment on `FreeThreadedStrategy` describing these use cases.
Fixes #125221.
Problem
Running failing SQL through
System.Data.OleDbcould crash the process withan
AccessViolationException/NullReferenceExceptioninComObject.Finalize()on the GC finalizer thread.
OleDbError's constructor creates source-generatedComObjectRCWs for theper-record
IErrorInfo/ISQLErrorInfoobjects. The non-[PreserveSig]extraction methods throw on a provider failure HRESULT, so a throw mid-ctor
skipped the manual release and left the RCW to be finalized. OLE DB error
objects are thread-affine, so releasing one on the finalizer thread faults.
Fix
Release the per-record
errorInfo/sqlErrorInfoRCWs intry/finallyso theyare always released on the calling thread, even when extraction throws.
Diagnostic
Also adds a DEBUG-only, Windows-only assert in
FreeThreadedStrategy: it probeseach COM object for agility (IAgileObject / free-threaded marshaler, mirroring
the built-in RCW) and
Debug.Asserts otherwise.[Conditional("DEBUG")], so itcompiles out of release builds. This would have caught the crash's root cause at
its source.
Note
This PR description was generated with the assistance of GitHub Copilot.